Skip to content

Conversation

@rentziass
Copy link
Contributor

Addresses #3137.

This essentially reverts 0858ed2, to avoid corrupting TLS sessions by directly reading from the underlying net.Conn. This is blocking upgrades to > 9.5.2 if using TLS.

conn_check.go is quite different on the v9.6 branch (it changed here), should I open another PR to fix that as well?

@rentziass
Copy link
Contributor Author

@ofekshenawa @vladvildanov sorry for the direct ping, would anyone of you be able to have a look at this please?

@vladvildanov
Copy link
Collaborator

@rentziass Hey! Thanks for reaching us out! Just for the context, currently it reads from TLS underlying socket, so it means that it reads unencrypted data?

@rentziass
Copy link
Contributor Author

@vladvildanov 👋 it's all in the issue but the tldr is that reading from (*tls.Conn).NetConn() directly corrupts TLS sessions (from NetConn() docs) and we found ourselves creating millions of connections versus the usual few thousands because of that. To be honest I'm surprised no other cluster user noticed this, it puts some noticeable pressure on the Redis cluster CPU. We thought it was an organic increase in load but it was just the constant re-creation of connections.

@vladvildanov vladvildanov merged commit e786862 into redis:master Oct 7, 2024
10 checks passed
rentziass added a commit to rentziass/go-redis that referenced this pull request Oct 7, 2024
@rentziass rentziass mentioned this pull request Oct 7, 2024
vladvildanov pushed a commit that referenced this pull request Oct 9, 2024
vladvildanov pushed a commit to vladvildanov/go-redis that referenced this pull request Oct 14, 2024
vladvildanov added a commit that referenced this pull request Oct 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants